fix: ship dual ESM/CJS output for Node16 consumers#373
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
This PR updates the build/publish setup to ship dual ESM+CJS artifacts (and corresponding declaration outputs) to improve TypeScript compatibility for consumers using module/moduleResolution: node16 in CommonJS projects, and adds a regression test that exercises the packed-as-published output.
Changes:
- Build
dist/in bothesmandcjsformats viatsdownto produce.mjs/.d.mtsand.cjs/.d.ctsoutputs. - Adjust package entrypoints/exports to publish separate
requireandimportconditions. - Add a Vitest regression test that
pnpm packs the package, installs it into a temporary Node16-resolution TS consumer, and type-checks it.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tsdown.config.ts | Switches tsdown output to dual ['esm', 'cjs'] formats. |
| tsconfig.json | Sets rootDir to address a pre-existing TS2209 and keep test exits clean. |
| scripts/package-exports.test.ts | Adds a pack-and-typecheck regression test for published exports/types behavior. |
| package.json | Updates entrypoints (main, types) and publish-time conditional exports for CJS/ESM. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Packing goes through `pnpm pack` because it applies the publishConfig | ||
| * overrides (exports/main/types), producing the same package.json a consumer | ||
| * gets from the registry. Lifecycle scripts are disabled: the prepack script | ||
| * runs a full build, which must not happen inside the test. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/package-exports.test.ts">
<violation number="1" location="scripts/package-exports.test.ts:39">
P3: The JSDoc mentions that `pnpm pack` applies publishConfig overrides for `exports/main/types`, but `publishConfig` in this project only defines `exports`. The `main` and `types` fields are top-level package.json entries, not publishConfig overrides. This makes the comment misleading about what actually changes in the packed tarball versus the source package.json — consider updating it to just reference `exports`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Changes build output from ESM-only to dual ESM/CJS and updates package exports. Moderate impact on package distribution; requires human review to verify no edge cases.
Re-trigger cubic
shashi-stackone
changed the title
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Auto-approved: Build configuration and test-only changes: dual ESM/CJS output, entry points, and a regression test. No source logic modified.
Re-trigger cubic
shashi-stackone
changed the title
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Auto-approved: Build configuration updates for dual ESM/CJS output with regression tests; no business logic changes.
Re-trigger cubic
Note: dual-format packages carry the standard dual-package hazard — a process mixing require() and import() of this package gets two class copies, so cross-boundary instanceof checks can fail; discriminate via err.name or statusCode in that situation.
Fixes #350
Summary by cubic
Ship dual ESM/CJS output so Node16 TypeScript consumers can use both import and require. Fixes TS1479 in CommonJS projects and resolves #350.
Bug Fixes
dist/as ESM and CJS viatsdown, emitting.mjs/.d.mtsand.cjs/.d.cts; set"main"to./dist/index.cjs,"module"to./dist/index.mjs,"types"to./dist/index.d.cts, and addpublishConfig.exportswithrequire/importconditions.pnpm --config.ignore-scripts=true pack, installs into a throwaway Node16 consumer, type-checks.cts/.mtswithtsc, smoke-testsrequire()/import(), verifiespublishConfig.exports["."], skips build-output checks whendist/is missing, and usesfileURLToPath(new URL(..., import.meta.url))instead ofimport.meta.dirname."rootDir"intsconfig.jsonto fix a pre-existing TS2209.Migration
require()andimport()of@stackone/ai, avoid cross-boundaryinstanceof; prefer discriminators likeerr.nameorstatusCode.Written for commit 53beb9a. Summary will update on new commits.